feat(mcp): add workspace parameter to write_note for parity with edit_note#964
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88eb2715d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "Use either workspace='workspace' with project='project', " | ||
| "or project='workspace/project', not both" | ||
| ) | ||
| return f"{cleaned_workspace}/{cleaned_project}" |
There was a problem hiding this comment.
When callers use the newly advertised workspace parameter with a workspace name or tenant_id, this helper concatenates that raw value into workspace/project, but get_project_client() later routes qualified project identifiers through resolve_workspace_project_identifier(), which matches the first segment only against WorkspaceInfo.slug (project_context.py lines 819-827). In cloud workspaces where the slug differs from the display name or tenant UUID, write_note(workspace=<name-or-tenant_id>, project=...) now fails with “Workspace ... was not found” even though the docstring says those identifiers are accepted; resolve the workspace identifier to its slug/tenant before building the route or restrict the accepted input to slugs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch on the resolver behavior, and the technical analysis is exactly right: resolve_workspace_project_identifier() (project_context.py:819-827) matches the first segment of a composed workspace/project route only against WorkspaceInfo.slug (casefold), with no fallback to the display name or tenant_id. So a composed route built from a name or tenant UUID does fail with "Workspace ... was not found", and the "slug, name, or tenant_id" wording over-promises.
One scope note before fixing it here: that wording is not new in this PR. It is byte-for-byte identical to the already-shipped edit_note — both the docstring (edit_note.py:368, write_note.py:126) and the _compose_* ValueError (edit_note.py:149, write_note.py:42) carry the same text. This PR's explicit goal is parity with edit_note, so correcting only write_note would make the two tools diverge for the same underlying behavior.
Since the inaccuracy affects edit_note equally, the right fix is a follow-up that touches both tools together — either tightening the docstring/error wording to "workspace slug" or teaching the resolver to accept names/tenant_ids (real resolution logic, also shared). Keeping this PR as a faithful copy and addressing both tools in one follow-up avoids divergence. Leaving the code unchanged here for that reason.
There was a problem hiding this comment.
Addressed in follow-up PR #979 (#979): rather than walking back the docs, resolve_workspace_project_identifier() now resolves the workspace segment by slug (casefold), then tenant_id (exact), then display name (casefold), with fail-fast ambiguity handling on non-unique names and slug-owner precedence. This makes the documented "slug, name, or tenant_id" contract real for edit_note today and for write_note once this PR merges. Scoped to the resolver + tests so it does not conflict with this branch.
… project routes The edit_note docstring (and write_note in PR #964) advertise that the workspace segment of a "workspace/project" route may be a slug, name, or tenant_id, but resolve_workspace_project_identifier() only matched against WorkspaceInfo.slug. Cloud routes using a display name or tenant UUID failed with "Workspace ... was not found" despite the documented contract. Extend the first-segment matching (only the matching logic; the overall resolution flow is unchanged) to honor, in priority order: 1. slug (casefold) — unchanged, checked first so working routes keep meaning 2. tenant_id — exact match on the opaque id 3. display name (casefold) — fails fast on collisions, listing candidate slugs A name that collides with another workspace's slug resolves to the slug owner. Unknown identifiers raise a not-found error naming the forms that were tried. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Drew Cain <groksrc@gmail.com>
…_note write_note now accepts workspace= alongside project= and project_id=, matching the parameter surface and docstring guidance already present in edit_note. The _compose_workspace_project_route helper is added locally (mirroring the edit_note pattern) so agents can route writes to same-named projects across workspaces using workspace/project qualified syntax. Closes #882 Other write-path tools that share the same gap (move_note, delete_note) are noted here but left for a separate PR to keep this change focused. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Drew Cain <groksrc@gmail.com>
The signature contract test pins exact tool parameters; update it for the new workspace parameter. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Drew Cain <groksrc@gmail.com>
groksrc
force-pushed
the
feat/882-write-note-workspace-targeting
branch
from
88eb271 to
19a70da
Compare
1 participant
Closes #882
Summary
Adds a
workspaceparameter towrite_note, bringing it to parity withedit_note's existing workspace-targeting support. Before this change, callers who needed to target a specific cloud workspace had to useedit_noteas a workaround;write_notesilently ignored any workspace context.What changed
src/basic_memory/mcp/tools/write_note.py_compose_workspace_project_routeprivate helper (identical logic to the one inedit_note.py) that composes aworkspace/projectroute whenworkspace=is supplied, or falls through to plain project/project_id routing otherwise.workspace: Optional[str] = Noneparameter to thewrite_notetool function.workspaceentry to the docstring Args section._compose_workspace_project_route(workspace=workspace, project=project, project_id=project_id)before the logfire span — exactly mirroringedit_note's pattern.tests/mcp/test_tool_write_note.py_compose_workspace_project_route.workspace=parameter via the full tool.Out of scope
Other tools with the same gap (
move_note,delete_note) were intentionally left out of this PR per the issue brief. They are candidates for a follow-up.How it was tested
workspace=kwarg present.write_notetest suite continues to pass unchanged.Review notes
test_write_note_accepts_workspace_paramis misleading: its docstring says "write_note routes correctly when workspace= is passed alongside project=", but the test body only passesproject=test_project.nameand never passesworkspace=. It is effectively a duplicate of the vanillawrite_notehappy-path test. The inline comment even acknowledges it only "confirms the parameter is accepted". Consider renaming or actually passing a validworkspace=.No positive end-to-end integration test proves a valid
workspace='foo'+project='bar'composes'foo/bar'and routes through the full tool.edit_note's suite has such a cloud-routing test (tests/mcp/test_tool_edit_note.py~line 1100 usingworkspace=personal). The new behavior's success path is only covered at the unit-helper level here. Low risk since the helper is byte-identical to the shippededit_notehelper and the wiring is verified, but it is a coverage gap vs. true parity.The helper is duplicated verbatim into
write_note.pyrather than shared. There are now two byte-identical copies of_compose_workspace_project_route(edit_note.pyandwrite_note.py) that must be kept in sync; if more tools adopt this (read_note/move_note/delete_noteper the issue's "ideally" audit), the duplication compounds. The GHA triage bot's own analysis recommended extracting it toproject_context.pyas a public helper. The implementer chose to mirroredit_note's private-helper pattern instead, which is a defensible minimal-diff choice but leaves a small DRY debt.🤖 Generated with Claude Code